-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Small cleanup and fixes #76
Conversation
libzecale/zecale_constants.hpp
Outdated
@@ -2,16 +2,21 @@ | |||
// | |||
// SPDX-License-Identifier: LGPL-3.0+ | |||
|
|||
#ifndef __ZECALE_ZECALE_CONSTANTS__ | |||
#define __ZECALE_ZECALE_CONSTANTS__ | |||
#ifndef __ZECALE_ZECALE_CONSTANTS_HPP__ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
libzecale should probably be completely agnostic to these constants, so maybe they would be better in the aggregator_server directory?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I followed a similar approach to what we did in Zeth: https://github.com/clearmatics/zeth/blob/develop/libzeth/zeth_constants.hpp here.
I think moving these in the aggregator_server directory is a good idea, but we need to find the boundary between what constitutes a "protocol param" and what's part of the backend/server config (that also applies to the Zeth-side then (but only these 3 vars would be potentially moved: https://github.com/clearmatics/zeth/blob/develop/libzeth/zeth_constants.hpp#L13-L16 I guess)).
Here (in Zecale), the libzecale::ZECALE_NUM_INPUTS_PER_NESTED_PROOF
definitely is a protocol param to me (as part of the protocol spec assumes underlying app hash their instance). The batch size however may be seen as a server config (along the lines of #75)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TBH - I would almost be tempted to move these to the CMakeLists file (along with the curve selection) to define the protocol params in the build config...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, makes sense to move those 3 constants in zeth as well.
No problem with the batch size etc being defined by CMake if thats easy enough. It would be ideal if they were out of scope during the build of libzecale (similaly for libzeth), i.e. it could be good to put them somewhere other than build/zecale.h. But that can be fixed up later if it's awkward.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As noted offline by @AntoineRondelet, libzecale::ZECALE_NUM_INPUTS_PER_NESTED_PROOF
is not necessarily a simple independent constant. I'll create an issue to resolve this in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll create an issue to resolve this in the future.
Thanks @dtebbs
As per our chat, I dropped the commit that used the libzecale::
constants in the aggregator_server to avoid "non-trivial" changes before v0.4. This reverts to the use of the static const
s in the aggregator_server.cpp. I also removed the libzecale/zecale_constants.hpp
file which was unused as of now. Let's see if we need to re-introduce it later on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great. For now, I've created #79 to resolve this later.
6452530
to
0ed4630
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
No description provided.